Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(backend): Caching should not be wrongly shown as successful if the artifacts have been deleted from S3 #7938

Closed
wants to merge 17 commits into from

Conversation

juliusvonkohout
Copy link
Member

@juliusvonkohout juliusvonkohout commented Jun 24, 2022

Description of your changes:

Fixes #7939

Checklist:

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented Jun 28, 2022

Maybe we should remove minio entirely to get rid of any possible licence issues. We could fetch the artifact as the persistenceagent does

artifactRequest := &api.ReadArtifactRequest{
RunId: runID,
NodeId: nodeStatus.ID,
ArtifactName: metricsArtifactName,
}
artifactResponse, err := r.pipelineClient.ReadArtifact(artifactRequest)
if err != nil {
return "", err
}
if artifactResponse == nil || artifactResponse.GetData() == nil || len(artifactResponse.GetData()) == 0 {
// If artifact is not found or empty content, skip the reporting.
return "", nil
}
archivedFiles, err := util.ExtractTgz(string(artifactResponse.GetData()))
if err != nil {
// Invalid tgz file. This should never happen unless there is a bug in the system and
// it is a unrecoverable error.
with a readartifact call instead. Then we would save a lot of S3 configuration too.

The apiserver S3 is configurable via environment variables

const (
minioServiceHost = "MINIO_SERVICE_SERVICE_HOST"
minioServicePort = "MINIO_SERVICE_SERVICE_PORT"
minioServiceRegion = "MINIO_SERVICE_REGION"
minioServiceSecure = "MINIO_SERVICE_SECURE"
pipelineBucketName = "MINIO_PIPELINE_BUCKET_NAME"
pipelinePath = "MINIO_PIPELINE_PATH"
mysqlServiceHost = "DBConfig.Host"
mysqlServicePort = "DBConfig.Port"
and since even the persistenceagent uses readArtifact we are anyway limited to use the S3 configuration of the apiserver. Trying to get caching work with another S3 configuration would be nonsense then.

I created a new PR for that #7958 but sadly there is not enough data in the cachedb to get the parameters for the readartifact call. Therefore we have to stick with this approach, if we do not want to change the database entries.

@juliusvonkohout juliusvonkohout changed the title WIP: fix(backend): Caching should not be wrongly shown as successful if the artifacts have been deleted from S3 fix(backend): Caching should not be wrongly shown as successful if the artifacts have been deleted from S3 Jun 28, 2022
@juliusvonkohout
Copy link
Member Author

/assign @Linchin @zijianjoy

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from zijianjoy after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@juliusvonkohout
Copy link
Member Author

Sadly we have to be agnostic of the backend until the new unified storage architecture from #7725 (comment) is there. So i got #8270 in instead. V2 pipelines are not yet covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[backend] the cache server does not check whether the cached artifacts have been deleted
4 participants